Skip to content

feat(tui): warn when a proposed endpoint is not method- and path-scoped - #2938

Open
vyncint wants to merge 1 commit into
NVIDIA:mainfrom
vyncint:feat/tui-l4-scope-warning
Open

feat(tui): warn when a proposed endpoint is not method- and path-scoped#2938
vyncint wants to merge 1 commit into
NVIDIA:mainfrom
vyncint:feat/tui-l4-scope-warning

Conversation

@vyncint

@vyncint vyncint commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

The draft inbox renders an endpoint's scope but never says when that scope is wide. An L4 tag or an allow * * is displayed exactly like a tightly scoped rule, so a reviewer approving quickly has nothing drawing their eye to a proposal that grants far more than the request that was denied. This adds a warning under the offending endpoint in the detail popup.

Related Issue

Part of #1098 — Definition of Done item "TUI shows an explicit L4/no-method-path scoping warning when applicable."

Not Fixes/Closes: this completes one of the seven DoD items. #2908 completed the rejected-guidance item; the remaining gaps are human_summary as the headline, which needs a proto field or a decision to fall back to rule_name.

While checking what was left I found that the intent_summary item already appears satisfied: crates/openshell-supervisor-network/src/policy_local.rs:1050 sets rationale: intent_summary, and the popup has rendered Rationale: for some time. That box looks tickable without code — flagging rather than assuming.

Changes

All in crates/openshell-tui/src/ui/sandbox_draft.rs (+144, −0):

  • scope_warning(&NetworkEndpoint) -> Option<&'static str> — a pure classifier next to endpoint_layer_label, which already carries the L4 test.
  • Rendered in the detail popup beneath the endpoint it describes, indented under it and styled like the existing security-note line.

It flags three cases:

Case Why
protocol empty (L4) the whole TCP port is allowed, not the denied request
REST with no allow rules nothing constrains method or path
REST allow rule with method or path unset format_allow_rule already renders these as *, but nothing flagged them

Protocols other than REST scope on command rather than method and path, so they are deliberately left alone rather than warned about incorrectly.

This surfaces guidance the project already gives rather than inventing a rule: docs/sandboxes/policy-advisor.mdx says "For REST APIs, prefer L7 rules over broad L4 access. A good proposal allows one method and the smallest safe path." The same page notes the mechanistic mapper drafts an L4 endpoint whenever no L7 samples are available, so broad proposals arrive by normal means and are worth flagging at review time. The wording is descriptive rather than an error.

Why the TUI and not the gateway

generate_security_notes (crates/openshell-server/src/grpc/policy.rs:5897) flags uninspected credentials, internal destinations, wildcard hosts, private and hostless allowed_ips, and well-known database ports — but nothing about L4 breadth or a missing method/path. Adding it there would change what the prover-adjacent notes mean and would affect auto-approval eligibility, which is a policy decision rather than a presentation one. Everything needed to classify this is already in proposed_rule.endpoints on the client, so this stays presentation-only. Happy to move it server-side if you would rather it be part of the security notes.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable) — not applicable: presentation-only inside openshell-tui, which has no e2e surface
Command Result
mise run ci exit 0
cargo test -p openshell-tui 77 passed, 0 failed

Eight new tests: five on the classifier (L4, REST without rules, unset method, unset path, fully scoped, non-REST left alone) and two render tests at 80×24 asserting the warning appears for an L4 endpoint and is absent for a scoped one.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — not applicable: no architecture, proto, or API change

Neighbours checked

#2935 is open on docs/sandboxes/policy-advisor.mdx as well, but its hunks are at lines 131 and 172 while this adds at 216, and it touches no openshell-tui file. Its advisor_proposed provenance change does not affect this classifier, which reads only protocol and rules. #2168 remains a stale draft in sandbox_draft.rs; nothing here touches approval_annotation or validation_issue_summary.

The draft inbox renders an endpoint's scope but never says when that scope is
wide. endpoint_layer_label already tags L4 and format_allow_rule already renders
an unset method or path as *, so the breadth is displayed while nothing draws a
reviewer's eye to a proposal that grants far more than the request that was
denied.

Add scope_warning, a pure classifier beside endpoint_layer_label, and render it
under the endpoint it describes in the detail popup, styled like the existing
security-note line. It flags an L4 endpoint, a REST endpoint with no allow
rules, and a REST allow rule that leaves the method or path unset. Protocols
other than REST scope on command rather than method and path, so they are left
alone rather than warned about incorrectly.

This stays in the TUI rather than in generate_security_notes because the
gateway's security notes feed auto-approval eligibility, so adding a case there
is a policy decision rather than a presentation one. Everything needed to
classify the endpoint is already on the client in proposed_rule.endpoints.

Part of NVIDIA#1098.

Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant